Show container start guidance on more commands#168
Conversation
gtsiolis
requested review from
anisaoshafi,
carole-lavillonniere and
silv-io
as code owners
📝 WalkthroughWalkthroughThe PR adds early runtime health checks to the logs, stop, and status commands to fail immediately if Docker is unavailable, and refactors error output formatting to use styled icons (✗ marker) and blockquote-style summaries instead of plain text prefixes. Changes
Sequence DiagramsequenceDiagram
participant Cmd as Command (logs/stop)
participant RT as Docker Runtime
participant HC as Health Checker
participant Out as Output Sink
participant Err as Error Handler
Cmd->>RT: NewDockerRuntime(cfg)
RT-->>Cmd: runtime instance
Cmd->>HC: checkRuntimeHealth(ctx, rt, cfg)
HC->>RT: IsHealthy(ctx)
alt Runtime is Unhealthy
RT-->>HC: error
HC->>RT: EmitUnhealthyError(error)
RT->>Out: emit styled error event
Out-->>Out: render with ✗ icon & blockquote
HC-->>Cmd: SilentError(wrapped)
Cmd->>Err: abort early
else Runtime is Healthy
RT-->>HC: nil
HC-->>Cmd: nil
Cmd->>Cmd: proceed to load config
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/root.go (1)
188-197: Comment is misleading andcfgparameter is unused.The comment states errors are emitted "if not in interactive mode," but the implementation always emits to
PlainSink(os.Stdout)regardless of mode—which aligns with the PR's goal of showing errors consistently. However, this makes thecfgparameter unused and the comment inaccurate.Consider either:
- Removing
cfgfrom the signature and updating the comment to reflect actual behavior, or- If future differentiation is planned, add a TODO comment.
✏️ Suggested comment fix (if cfg removal is desired)
-// checkRuntimeHealth checks if the runtime is healthy and emits an error -// through the sink if not in interactive mode. Returns a SilentError to -// suppress duplicate error printing. -func checkRuntimeHealth(ctx context.Context, rt runtime.Runtime, cfg *env.Env) error { +// checkRuntimeHealth checks if the runtime is healthy and emits an error +// through a plain sink. Returns a SilentError to suppress duplicate error printing. +func checkRuntimeHealth(ctx context.Context, rt runtime.Runtime) error { if err := rt.IsHealthy(ctx); err != nil { rt.EmitUnhealthyError(output.NewPlainSink(os.Stdout), err) return output.NewSilentError(err) } return nil }This would also require updating call sites in
logs.go,stop.go, andstatus.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/root.go` around lines 188 - 197, The function checkRuntimeHealth has an unused cfg parameter and an inaccurate comment; remove the cfg *env.Env* parameter from checkRuntimeHealth's signature, update its comment to state it always emits the runtime unhealthy error to a PlainSink(os.Stdout) and returns a SilentError, and then update every call site of checkRuntimeHealth to stop passing cfg (i.e., adjust callers to call checkRuntimeHealth(ctx, rt)); run tests/build to ensure no remaining references.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/status.go`:
- Around line 26-40: The NewDockerRuntime failure handling in status.go is
inconsistent with logs.go and stop.go and omits the OS-specific action provided
by EmitUnhealthyError; extract a shared helper (e.g., initAndCheckRuntime or
EnsureHealthyRuntime) that encapsulates runtime.NewDockerRuntime(cfg.DockerHost)
plus the health-check via checkRuntimeHealth and when NewDockerRuntime returns
an error calls the same styled EmitUnhealthyError/EmitError path used elsewhere
(mirroring EmitUnhealthyError's actions and message), then replace the
NewDockerRuntime + checkRuntimeHealth logic in status.go, logs.go, and stop.go
with calls to this helper so all commands present identical, OS-aware error
output.
In `@internal/output/plain_format.go`:
- Around line 153-164: The test suite lacks assertions verifying visual styling
differences between the first and subsequent actions; update
TestErrorDisplay_MultiActionRenders to assert that the rendered output contains
styling markers (or ANSI sequences) produced by errorActionStyle and
errorValueStyle for the first action and that subsequent actions include the
errorMutedStyle output (e.g., by checking for the specific ANSI color/bold
sequences or unique style markers applied by errorActionStyle/errorValueStyle vs
errorMutedStyle in the output string derived from rendering e.Actions); locate
rendering logic around e.Actions in plain_format.go (errorActionStyle,
errorValueStyle, errorMutedStyle) and enhance the test to explicitly assert the
first action uses the action+value styles while later actions use the muted
style.
---
Nitpick comments:
In `@cmd/root.go`:
- Around line 188-197: The function checkRuntimeHealth has an unused cfg
parameter and an inaccurate comment; remove the cfg *env.Env* parameter from
checkRuntimeHealth's signature, update its comment to state it always emits the
runtime unhealthy error to a PlainSink(os.Stdout) and returns a SilentError, and
then update every call site of checkRuntimeHealth to stop passing cfg (i.e.,
adjust callers to call checkRuntimeHealth(ctx, rt)); run tests/build to ensure
no remaining references.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 04e4a18d-8ab3-4aaa-bfd5-9c04aa757e7b
📒 Files selected for processing (7)
cmd/logs.gocmd/root.gocmd/status.gocmd/stop.gointernal/output/plain_format.gointernal/output/plain_format_test.gointernal/output/plain_sink_test.go
| sb.WriteString(errorActionStyle.Render(ErrorActionPrefix+action.Label+" ")) | ||
| sb.WriteString(errorValueStyle.Render(action.Value)) | ||
| } | ||
| } |
There was a problem hiding this comment.
question: I would like to hear what @silv-io says here. We were previously not doing any styling in non-interactive mode.
There was a problem hiding this comment.
The plain output should remain styling free IMO. If we want a non-interactive styled output I think that should be a separate sink
There was a problem hiding this comment.
Why do you think so?
Right now it's binary: interactive TTY gets Bubble Tea, everything else gets plain. A third styled-but-non-interactive sink would need a trigger. What would it be? CI is a good example: no TTY, so non-interactive, but GitHub Actions and most CI systems do render ANSI colors fine.
There was a problem hiding this comment.
the trigger word there is "most" for me. I don't want to force color onto systems where it might not be rendered well. If we add color styling to the plain output, we should at the least check that it will handle NO_COLOR correctly and add a test for that.
Also, then I think the styling should live outside of the UI package and in its own "style" package to avoid potential circular dependencies in the future.
There was a problem hiding this comment.
I think for now we could just remove colors/styling from plain sink and revisit later if we need a third kind of sink.
Fine for you @gtsiolis?
I'm wondering if this was intentional in the first place?
There was a problem hiding this comment.
The intention was to add similar UX for both start and status commands when Docker is not detected.
| start | status |
|---|---|
 |
 |
Added 45f92f0 to try how the extra sink would look like, what do you think?
Happy to drop the colors for now. Easy revert.
gtsiolis
force-pushed
the
george/des-187
branch
2 times, most recently
from
ab95f7d to
e78724f
Compare
gtsiolis
force-pushed
the
george/des-187
branch
from
e78724f to
45f92f0
Compare
3 participants
Show actionable Docker errors consistently across all commands
When Docker isn't available, status, stop, and logs now all show a clear, styled error message with actionable next steps — regardless of whether the terminal is interactive or not.
Previously the error guidance was only shown in non-interactive mode (piped output), so running lstk status in a terminal with Docker stopped produced no visible output. The plain formatter also rendered errors as unstyled text even when colors were available.